Fix N+1 query pattern in bulk pool delete endpoint by ColtenOuO · Pull Request #66222 · apache/airflow

ColtenOuO · 2026-05-01T19:13:54Z

Summary

Eliminates an N+1 query in the bulk pool delete endpoint by reusing the ORM objects already loaded by BulkPoolService.categorize_pools() instead of re-fetching each pool one at a time inside the delete loop.

Why

categorize_pools() already loads every requested pool with a single batched SELECT ... WHERE pool IN (...) and returns them as existing_pools_dict: dict[str, Pool]. handle_bulk_delete() was discarding that dict (_, matched, not_found = ...) and then re-querying each pool individually inside the loop:

for pool_name in delete_pool_names:
    existing_pool = self.session.scalar(select(Pool).where(Pool.pool == pool_name).limit(1))
    ...

For a request that deletes N pools, this produced 2N + 1 round-trips to the database (1 batched SELECT in categorize_pools + N redundant per-pool SELECTs + N DELETEs). The redundant per-pool SELECTs return objects we already have in memory.

Change 1

Behavior is preserved exactly: existing_pools_dict.get(pool_name) returns the same Pool instance the per-pool SELECT would have returned (it's the same object loaded by categorize_pools in the same session), and is None when the pool doesn't exist — matching the original session.scalar(...).limit(1) semantics.

Change 2

Added a unit test that asserts the expected query count.

Impact

Bulk size	Before (queries)	After (queries)	Saved
10 pools	21	11	-48%
100 pools	201	101	-50%

Unit Test Results

To ensure this optimization doesn't introduce any regressions, I've verified the changes with the existing test suite. All tests passed.

Command:
uv run --project airflow-core pytest airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py::TestBulkPools -x

Was generative AI tooling used to co-author this PR?

Yes — (Claude Sonnet 4.6, for test_bulk_delete_does_not_re_query_each_pool in airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py‎)

SameerMesiah97

Solid performance improvement.

ColtenOuO · 2026-05-02T07:22:14Z

I’ve noted that handle_bulk_update and update_orm_from_pydantic in pools.py likely suffer from similar N+1 query issues. I've intentionally kept this PR focused to verify if this optimization approach aligns with the project's expectations.

Once this logic is confirmed and merged, I will follow up with another PR to address the remaining functions. Thanks again to all the reviewers!

jason810496

LGTM overall, thanks for the improvement.
It would be nice to add or edit corresponding test case to guard the query count.

FYI, the assert_queries_count is the current convention to check the expected query count.

airflow/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_dags.py

Lines 428 to 441 in 7336ed4

    
           def test_get_dags(self, test_client, query_params, expected_total_entries, expected_ids, session): 
        
               # Only create asset test data for asset-related tests to avoid affecting other tests 
        
               if any(param in query_params for param in ["has_asset_schedule", "asset_dependency"]): 
        
                   self._create_asset_test_data(session) 
        
               with assert_queries_count(4): 
        
                   response = test_client.get("/dags", params=query_params) 
        
               assert response.status_code == 200 
        
               body = response.json() 
        
               assert body["total_entries"] == expected_total_entries 
        
               actual_ids = [dag["dag_id"] for dag in body["dags"]] 
        
               assert actual_ids == expected_ids

ColtenOuO · 2026-05-02T20:26:33Z

Thanks for the suggestion @jason810496 — I've added a test that uses assert_queries_count per the convention you linked.

Additionally, while checking this unit test, I realized that using round-trip times provides a more accurate evaluation of the impact. I have updated the table below to reflect this:

Bulk size	Before	After	Saved
10 pools	14	4	−71%
100 pools	104	4	−96%

If you have any feedback or suggestions for this PR, feel free to let me know! Thanks to all the reviewers for your support!

henry3260

Thanks for your effort!

henry3260

Thanks! I think we are almost there

jason810496

LGTM overall, one final nit before merge. Thanks.

jason810496

Would it be better to simplify as accepting single pool_count? Since the expected count should be the same.

ColtenOuO · 2026-05-17T17:55:55Z

Thanks for the feedback @jason810496!

Refactored the code based on your suggestion. I initially over-engineered it to avoid the hardcoded query count.

Feel free to let me know if there’s anything else or if I misunderstood anything.

ColtenOuO · 2026-05-17T18:44:33Z

Fixed ruff formatting to pass CI (force-pushed to the latest commit).

potiuk · 2026-05-18T10:52:16Z

@ColtenOuO — There are 4 unresolved review threads on this PR from @jason810496. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks!

Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

jason810496 · 2026-05-18T11:36:46Z

Thanks for the fix.

github-actions · 2026-05-18T11:38:17Z

Backport successfully created: v3-2-test

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

Status	Branch	Result
✅	v3-2-test

… (#67108) * Fix N+1 query pattern in bulk pool delete endpoint * Add query-count test for bulk pool delete * Refactor bulk-delete query-count test to parametrize * Parametrize bulk-delete query-count test by pool size (cherry picked from commit 91806fd) Co-authored-by: Colten <jun930436@gmail.com>

ColtenOuO requested review from bugraoz93, choo121600, ephraimbuddy, jason810496, pierrejeambrun, rawwar and shubhamraj-git as code owners May 1, 2026 19:13

boring-cyborg Bot added the area:API Airflow's REST/HTTP API label May 1, 2026

SameerMesiah97 approved these changes May 1, 2026

View reviewed changes

jason810496 reviewed May 2, 2026

View reviewed changes

henry3260 reviewed May 3, 2026

View reviewed changes

Comment thread airflow-core/src/airflow/api_fastapi/core_api/services/public/pools.py Outdated

Fix N+1 query pattern in bulk pool delete endpoint

48533f3

ColtenOuO force-pushed the fix-bulk-pool-delete-n-plus-one branch from f7055e7 to 1a6ae94 Compare May 3, 2026 11:31

ColtenOuO mentioned this pull request May 3, 2026

Fix backfill validate n plus one #66313

Open

1 task

potiuk added the ready for maintainer review Set after triaging when all criteria pass. label May 5, 2026

ColtenOuO requested a review from henry3260 May 15, 2026 17:29

henry3260 reviewed May 15, 2026

View reviewed changes

Comment thread airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py Outdated

eladkal added this to the Airflow 3.2.2 milestone May 16, 2026

eladkal added type:bug-fix Changelog: Bug Fixes backport-to-v3-2-test Mark PR with this label to backport to v3-2-test branch labels May 16, 2026

Add query-count test for bulk pool delete

6ded2bf

ColtenOuO force-pushed the fix-bulk-pool-delete-n-plus-one branch from 1a6ae94 to 6ded2bf Compare May 17, 2026 08:04

jason810496 reviewed May 17, 2026

View reviewed changes

Comment thread airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py Outdated

Refactor bulk-delete query-count test to parametrize

c1542bc

ColtenOuO requested review from henry3260 and jason810496 May 17, 2026 10:03

jason810496 reviewed May 17, 2026

View reviewed changes

Comment thread airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py Outdated

Comment thread airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py Outdated

Comment thread airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py Outdated

ColtenOuO requested a review from jason810496 May 17, 2026 17:56

Parametrize bulk-delete query-count test by pool size

69fc441

ColtenOuO force-pushed the fix-bulk-pool-delete-n-plus-one branch from 7a4cc2d to 69fc441 Compare May 17, 2026 18:42

vatsrahul1001 modified the milestones: Airflow 3.2.2, Airflow 3.3.0 May 18, 2026

jason810496 approved these changes May 18, 2026

View reviewed changes

jason810496 merged commit 91806fd into apache:main May 18, 2026
143 checks passed

vatsrahul1001 modified the milestones: Airflow 3.3.0, Airflow 3.2.2 May 18, 2026

jason810496 mentioned this pull request May 22, 2026

Fix N+1 query in bulk task instance delete endpoint #67304

Open

1 task

	def test_get_dags(self, test_client, query_params, expected_total_entries, expected_ids, session):
	# Only create asset test data for asset-related tests to avoid affecting other tests
	if any(param in query_params for param in ["has_asset_schedule", "asset_dependency"]):
	self._create_asset_test_data(session)

	with assert_queries_count(4):
	response = test_client.get("/dags", params=query_params)
	assert response.status_code == 200
	body = response.json()

	assert body["total_entries"] == expected_total_entries
	actual_ids = [dag["dag_id"] for dag in body["dags"]]

	assert actual_ids == expected_ids

Conversation

ColtenOuO commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why

Change 1

Change 2

Impact

Unit Test Results

Was generative AI tooling used to co-author this PR?

Uh oh!

SameerMesiah97 left a comment

Choose a reason for hiding this comment

Uh oh!

ColtenOuO commented May 2, 2026

Uh oh!

jason810496 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColtenOuO commented May 2, 2026

Uh oh!

henry3260 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

henry3260 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jason810496 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jason810496 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ColtenOuO commented May 17, 2026

Uh oh!

ColtenOuO commented May 17, 2026

Uh oh!

potiuk commented May 18, 2026

Uh oh!

jason810496 commented May 18, 2026

Uh oh!

Uh oh!

github-actions Bot commented May 18, 2026

Backport successfully created: v3-2-test

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ColtenOuO commented May 1, 2026 •

edited

Loading

jason810496 left a comment •

edited

Loading